Skip to content

drivers/sx126x add optional CONFIG_SX126X_DEFAULT_SYNC_WORD #21711

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

drivers/sx126x add optional CONFIG_SX126X_DEFAULT_SYNC_WORD #21711

wants to merge 1 commit into from
+19 −0

Conversation

@fabian18
Copy link
Contributor

Contribution description

Configure a LoRa sync word if CONFIG_SX126X_DEFAULT_SYNC_WORD is defined

Alternatively, should it be in params, in case 2 LoRa on a board should use different sync words?

Testing procedure

Issues/PRs references

#21675

@github-actions github-actions bot added Area: drivers Area: Device drivers Area: LoRa Area: LoRa radio support labels Sep 11, 2025
@crasbe crasbe added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Discussion: RFC The issue/PR is used as a discussion starting point about the item of the issue/PR CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Sep 11, 2025
@riot-ci
Copy link

riot-ci commented Sep 11, 2025 •
edited
Loading

Murdock results

✔️ PASSED

7bd175e drivers/sx126x: add optional CONFIG_SX126X_DEFAULT_SYNC_WORD

Success Failures Total Runtime
10515 0 10516 12m:52s

Artifacts

@benpicco benpicco requested a review from jia200x September 11, 2025 13:25
benpicco
benpicco reviewed Sep 11, 2025
View reviewed changes
@fabian18
Copy link
Contributor Author

fabian18 commented Sep 26, 2025 •
edited
Loading

Alternatively, should it be in params, in case 2 LoRa on a board should use different sync words?

This should be resolved. I think @maribu or @jia200x could have an opinion.

@benpicco
Copy link
Contributor

Please add some documentation as to what CONFIG_SX126X_DEFAULT_SYNC_WORD might be.

@fabian18 fabian18 force-pushed the pr/sx126x_radio_hal_sync_word branch from afe97d8 to 3aa6c33 Compare October 8, 2025 13:47
crasbe
crasbe reviewed Oct 8, 2025
View reviewed changes
@fabian18 fabian18 force-pushed the pr/sx126x_radio_hal_sync_word branch from 3aa6c33 to 450b653 Compare October 8, 2025 14:24
@crasbe crasbe removed the Discussion: RFC The issue/PR is used as a discussion starting point about the item of the issue/PR label Oct 8, 2025
crasbe
crasbe reviewed Oct 8, 2025
View reviewed changes
drivers/sx126x/include/sx126x_internal.h Outdated
Comment on lines 30 to 44
#if defined(DOXYGEN)
/**
* @brief Configure the LoRa sync word
*
* The sync word for sx126x is 16 bits long.
* Private networks should use 0x1424.
* Public networks should use 0x3444.
* Using the driver API you only configure 2 nibbles Y and Z to form a sync word 0xY4Z4.
* The default chip value is 0x12 (0x1424).
*
* See https://blog.classycode.com/lora-sync-word-compatibility-between-sx127x-and-sx126x-460324d1787a
* for more information and a comparison with sx127x.
*/
# define CONFIG_SX126X_DEFAULT_SYNC_WORD 0x12
#endif
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this macro supposed to be an "internal only" macro or is it expected that a user can (and might want to) change it?

You have to navigate from the SX126x main page...
image

...to the internal page to find it.
image

Maybe the sx126x.h or sx126x_params.h would be a better home?

Copy link
Contributor Author

@fabian18 fabian18 Oct 8, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would say every CONFIG_ may be set by a user. There are a few CONFIG_ in some c files, which means they do not appear in the Doxygen doc. I moved it to internal because I am going to move all the other CONFIG_ there as well.

There are 2 ways that make sense to me. Either all CONFIG_ go to a public header where it is more likely to be seen by a user. Or some (if not all) CONFIG_ macros go to internal headers because setting them requires more experienced knowledge. There is no strict rule at the moment. The user is likely a (student) developer, so I would say the user is also able to look at internal doc.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

moved to public header

@crasbe
Copy link
Contributor

crasbe commented Oct 8, 2025

Also small nitpick: the headline and commit message are missing the : after drivers/sx126x 👀

@fabian18
Copy link
Contributor Author

fabian18 commented Oct 8, 2025

So the commit message check lacks the check for the :

@crasbe crasbe mentioned this pull request Oct 8, 2025
Open
@fabian18 fabian18 force-pushed the pr/sx126x_radio_hal_sync_word branch from 450b653 to 7bd175e Compare October 9, 2025 07:17
@crasbe crasbe requested a review from benpicco October 14, 2025 22:59
@jia200x
Copy link
Member

jia200x commented Oct 16, 2025

This should be resolved. I think @maribu or @jia200x could have an opinion.

At best it should be a runtime configurable option. E.g a LoRaWAN device may join a private LoRaWAN network (sync word 0x39) or a public LoRaWAN network (sync word 0x34) without requiring to change the firmware

@fabian18
Copy link
Contributor Author

fabian18 commented Oct 17, 2025 •
edited
Loading

should be a runtime configurable option

This smells like a NETOPT.

benpicco
benpicco approved these changes Oct 26, 2025
View reviewed changes
Copy link
Contributor

@benpicco benpicco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding a NETOPT for the sync word would be a good follow-up, but setting the default value is independent of that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@crasbe crasbe crasbe left review comments

@benpicco benpicco benpicco approved these changes

@jia200x jia200x Awaiting requested review from jia200x

Assignees

No one assigned

Labels

Area: drivers Area: Device drivers Area: LoRa Area: LoRa radio support CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@fabian18 @riot-ci @benpicco @crasbe @jia200x